pg_lake_copy: reject multidimensional arrays in COPY TO before CSV serialization#430
Open
sfc-gh-mslot wants to merge 3 commits into
Open
pg_lake_copy: reject multidimensional arrays in COPY TO before CSV serialization#430sfc-gh-mslot wants to merge 3 commits into
sfc-gh-mslot wants to merge 3 commits into
Conversation
sfc-gh-mslot
commented
Jul 17, 2026
| * on a value we will reject. | ||
| */ | ||
| if (get_element_type(attr->atttypid) != InvalidOid && | ||
| cstate->targetFormat != DATA_FORMAT_ICEBERG) |
Collaborator
Author
There was a problem hiding this comment.
if it's handled upstream, do we need the cstate->targetFormat != DATA_FORMAT_ICEBERG check here?
…rialization COPY <table> TO '<lake-file>' serializes rows to an intermediate temp CSV and hands it to DuckDB for conversion to Parquet/JSON. The intermediate CSV path had no guard against multidimensional array values: a column declared int[] can hold ARRAY[[1,2],[3,4]] at runtime, which serializes to [[1,2],[3,4]] in the CSV. DuckDB cannot cast that string back to a flat LIST(T) and either returns a cryptic Conversion Error or -- for certain element types (uuid, bigint, numeric) -- crashes the shared pgduck_server process, disconnecting all concurrent sessions (issue #408). The Iceberg table write path already handles this via IcebergErrorOrClampMultiDimArrayDatum in iceberg_datum_validation.c; plain COPY TO had no equivalent guard. Fix: in CopyOneRowTo (csv_writer.c), after the existing numeric NaN/Inf checks, detect array-typed columns and raise ERRCODE_FEATURE_NOT_SUPPORTED when ARR_NDIM > 1, before the value is serialized to the CSV. The check is skipped for the Iceberg format (DATA_FORMAT_ICEBERG) where the upstream guardrail already applies. Also corrects a misleading comment in ConvertCSVFileTo (write_data.c) that described the ICEBERG_OOR_NONE sentinel as if clamping had already occurred for all callers; it is only true for the Iceberg FDW INSERT path. Tests: - test_copy_to_multidim_array_errors: verifies a clear pg_lake error is raised for a table with a multidim int[] value, with no engine involved - test_copy_to_1d_array_succeeds: regression guard confirming 1-D arrays still round-trip correctly after the check is added Fixes #407. Mitigates #408 (multidim values are now blocked before reaching DuckDB). Signed-off-by: Richie Bachala <richie.bachala@snowflake.com> Signed-off-by: Marco Slot <marco.slot@snowflake.com>
…check The multidimensional array check in CopyOneRowTo skipped DATA_FORMAT_ICEBERG, but no reachable path delivers a multidimensional value to the CSV writer with that target format: COPY TO in Iceberg format is rejected earlier in EnsureFormatSupported, and the Iceberg INSERT path already rejects or nullifies such values upstream via IcebergErrorOrClampDatum. Running the check for every format is a harmless backstop and simpler. Comment updated to match. Signed-off-by: Marco Slot <marco.slot@snowflake.com>
sfc-gh-mslot
force-pushed
the
marcoslot/copy-to-multidim-guardrail
branch
from
July 17, 2026 08:22
2ef6c93 to
41e33ce
Compare
The multidimensional-array guard added in CopyOneRowTo ran for every target format. That was broader than the bug it fixes: only formats that serialize the value through DuckDB as a typed LIST(T) (Parquet/Iceberg/ JSON) hit the failing cast (#407/#408). For CSV, ShouldUseDuckSerialization is false and ChooseDuckDBEngineTypeForWrite treats every column as VARCHAR, so a value is written using PostgreSQL array text ("{{1,2},{3,4}}") and copied through verbatim -- multidimensional arrays round-trip fine today. Rejecting them for CSV was therefore a behaviour regression for COPY <table> TO '<object-store-url>' WITH (format 'csv'). Gate the guard on useDuckSerialization so it fires only for the formats that actually cast to a list, and update the errhint to mention CSV as an escape hatch. Regression coverage: - test_copy_to_multidim_array_csv (test_csv_copy.py): multidim int[] to a CSV object-store file succeeds and is preserved as PG array text. - test_copy_to_multidim_array_json_errors (test_parquet_copy.py): JSON still rejects multidim arrays, locking in the guard for DuckDB-serialized formats. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Marco Slot <marco.slot@snowflake.com>
sfc-gh-mslot
force-pushed
the
marcoslot/copy-to-multidim-guardrail
branch
from
July 17, 2026 09:03
f23ccbf to
5621ac2
Compare
| * EnsureFormatSupported. In practice this only fires on | ||
| * plain COPY TO to a Parquet or JSON file. | ||
| */ | ||
| if (useDuckSerialization && |
Collaborator
There was a problem hiding this comment.
should we also disallow in arbitrarily nested structures? (can recurse as in #448 )
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased version of #428 (by @sfc-gh-rbachala) with the C indentation corrected, opened to run CI.
The original change is unmodified in substance. The only fix here is running
pgindentover the two edited C files: the added block inCopyOneRowToand the rewritten comment inConvertCSVFileToused space indentation, which fails thecheck-indentCI step. Reindenting restores the project's tab style, so the diff against main is now just the logical change (106 insertions, 2 deletions).Authorship is preserved: the commit is still authored by Richie Bachala with their
Signed-off-by, with my own sign-off added as committer.What the change does
COPY
<table>TO a lake file serializes rows through an intermediate CSV before handing them to DuckDB. That path had no guard against multidimensional array values, so a column declaredint[]holdingARRAY[[1,2],[3,4]]would serialize to[[1,2],[3,4]], which DuckDB cannot cast back to a flatLIST(T)— returning a cryptic Conversion Error or, for some element types, crashing the shared pgduck_server process (#408). The fix rejectsARR_NDIM > 1inCopyOneRowTobefore serialization, matching the guard the Iceberg write path already has.Fixes #407. Mitigates #408.
Test plan
test_copy_to_multidim_array_errors— a multidimint[]value raises a clear pg_lake error with no engine involvementtest_copy_to_1d_array_succeeds— 1-D arrays still round-trip correctlyCI on this branch exercises the full build, indentation check, and pytest suite.